test: HTTP/CLI route matrix and structured API error codes#42
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes API error handling with an ChangesStructured Error Codes and Comprehensive API/CLI Test Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/export_api.py (1)
287-317:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
export_session()parse/stats failures with structured error responses.This path still has unhandled exceptions, so 500s here won’t consistently return
{ "error", "code" }.Proposed fix
- fmt = request.args.get("format", "md") - session = parse_session(filepath) + fmt = request.args.get("format", "md") + try: + session = parse_session(filepath) + except Exception: + current_app.logger.exception("Failed to parse session %s", session_id) + return error_response( + ErrorCode.PARSE_ERROR, + "Failed to parse session", + 500, + ) + rules = current_app.config.get("EXCLUSION_RULES") or [] if is_session_excluded(rules, session, project_name): return error_response( ErrorCode.SESSION_NOT_FOUND, "Session not found", 404, ) - stats = compute_stats(session) + try: + stats = compute_stats(session) + except Exception: + current_app.logger.exception("Failed to compute stats for %s", session_id) + return error_response( + ErrorCode.INTERNAL_ERROR, + "Failed to compute session stats", + 500, + ) title_slug = slugify(session["title"], default="session")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/export_api.py` around lines 287 - 317, Wrap calls to parse_session(session filepath) and compute_stats(session) in try/except blocks inside export_session so any exceptions are caught and returned via the existing error_response(...) shape rather than bubbling as 500 HTMLs; specifically, catch exceptions from parse_session (and return a structured error_response with an appropriate ErrorCode like SESSION_NOT_FOUND or a new PARSE_ERROR and a 400/404) and catch exceptions from compute_stats (return ErrorCode.INTERNAL_ERROR or STATS_COMPUTE_FAILED with a 500), ensuring the rest of the flow (session_to_json/session_to_markdown and send_file) only runs when parsing and stats succeed.
🧹 Nitpick comments (4)
tests/test_api_routes.py (1)
68-72: ⚡ Quick winHarden capped-limit assertion with response-shape check.
This test can pass on non-list payloads (e.g., an unexpected JSON object) because it only checks
len(...) <= 500. Assert list shape explicitly before length checks.Proposed test hardening
def test_search_limit_capped_at_max(client): resp = client.get("/api/search?q=Hello&limit=9999") assert resp.status_code == 200 - assert len(resp.get_json()) <= 500 + body = resp.get_json() + assert isinstance(body, list) + assert len(body) <= 500🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_api_routes.py` around lines 68 - 72, The test_search_limit_capped_at_max test currently only checks len(resp.get_json()) <= 500 which will pass for non-list JSON shapes; update the test to first assert the response body is a JSON list (e.g., use resp.is_json and assert isinstance(resp.get_json(), list) or equivalent) before asserting its length, so the shape is validated prior to the capped-limit check in test_search_limit_capped_at_max.tests/test_search.py (1)
8-12: ⚡ Quick winTighten happy-path assertions to validate limit behavior.
These tests currently confirm status, but not enough of the response contract. Add explicit list-shape checks and verify requested/default limit constraints in assertions.
Proposed assertion upgrades
def test_limit_integer_string(client_single): resp = client_single.get("/api/search?q=Hello&limit=10") assert resp.status_code == 200 - assert isinstance(resp.get_json(), list) + body = resp.get_json() + assert isinstance(body, list) + assert len(body) <= 10 @@ def test_limit_default(client_single): resp = client_single.get("/api/search?q=Hello") assert resp.status_code == 200 + body = resp.get_json() + assert isinstance(body, list) + assert len(body) <= 200Also applies to: 26-29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_search.py` around lines 8 - 12, Update the happy-path assertions in test_limit_integer_string (and the similar test at lines 26-29) to assert the response is a list, each element has the expected shape (e.g., dict with expected keys) and that the number of returned items respects the requested or default limit: assert isinstance(resp.get_json(), list), assert all(isinstance(item, dict) and set(expected_keys).issubset(item.keys()) for item in resp.get_json()), and assert len(resp.get_json()) <= requested_limit (for "/api/search?q=Hello&limit=10" assert <= 10) and also add a case that verifies the default limit when limit is omitted by calling the endpoint without limit and asserting len(...) <= default_limit; use the test function names test_limit_integer_string and the other test to locate and update the assertions.tests/test_cli_e2e.py (2)
50-55: ⚡ Quick winTighten success assertion for listed project.
On Line 54,
or "Project" in proc.stdoutcan pass even whentest-projectis missing (header-only output). Assert the seeded project name directly to avoid false positives.Proposed diff
def test_cli_list_exits_zero(tmp_path): base = _seed_base_dir(tmp_path) proc = _run_cli(["list", "--base-dir", str(base)]) assert proc.returncode == 0 - assert "test-project" in proc.stdout or "Project" in proc.stdout + assert "test-project" in proc.stdout🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cli_e2e.py` around lines 50 - 55, The test test_cli_list_exits_zero uses a loose assertion that allows header-only output to pass; tighten it by removing the fallback check ("or 'Project' in proc.stdout") and assert directly that the seeded project name "test-project" appears in proc.stdout (use the existing proc from _run_cli in test_cli_list_exits_zero to perform the check).
70-73: ⚡ Quick winAssert the invalid
--sincefailure reason, not only exit code.Line 72 currently accepts any non-zero failure. Add a stderr assertion so the test proves the
--sincevalidation path specifically failed.Proposed diff
def test_cli_invalid_since_exits_nonzero(): proc = _run_cli(["--since", "yesterday"]) assert proc.returncode != 0 + assert "--since" in proc.stderr or "invalid choice" in proc.stderr.lower()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cli_e2e.py` around lines 70 - 73, The test test_cli_invalid_since_exits_nonzero only checks a non-zero exit code; update it to also assert that _run_cli([...])'s stderr contains the specific validation error for the --since flag (e.g., assert "Invalid value for '--since'" in proc.stderr or a substring like "--since" and "invalid" if exact text differs) so the test verifies the --since validation path failed; keep using the existing helper _run_cli to capture stdout/stderr and add the stderr assertion alongside the returncode check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/sessions.py`:
- Around line 65-75: The current single try around parse_session(filepath) and
compute_stats(session) masks parse failures as INTERNAL_ERROR; split the logic
so parse errors are caught and mapped to ErrorCode.PARSE_ERROR. Specifically,
call parse_session(filepath) in its own try/except that catches parsing
exceptions and returns error_response(ErrorCode.PARSE_ERROR, "Failed to parse
session", 400) (include session_id in logs via current_app.logger.exception),
then call compute_stats(session) in a separate try/except that logs and returns
the existing INTERNAL_ERROR via error_response(ErrorCode.INTERNAL_ERROR, "Failed
to compute session stats", 500) if compute_stats fails. Ensure you reference
parse_session, compute_stats, session_id, current_app.logger.exception,
error_response and ErrorCode.PARSE_ERROR in the change.
In `@tests/conftest.py`:
- Around line 22-24: The shared error helper in tests/conftest.py currently only
asserts "code" is present in body; add a type assertion to ensure body["code"]
is a string (e.g., use isinstance(body["code"], str)) before comparing with
expected_code in the helper that contains the assertions around "code" and
expected_code so tests fail if handlers return non-string codes.
---
Outside diff comments:
In `@api/export_api.py`:
- Around line 287-317: Wrap calls to parse_session(session filepath) and
compute_stats(session) in try/except blocks inside export_session so any
exceptions are caught and returned via the existing error_response(...) shape
rather than bubbling as 500 HTMLs; specifically, catch exceptions from
parse_session (and return a structured error_response with an appropriate
ErrorCode like SESSION_NOT_FOUND or a new PARSE_ERROR and a 400/404) and catch
exceptions from compute_stats (return ErrorCode.INTERNAL_ERROR or
STATS_COMPUTE_FAILED with a 500), ensuring the rest of the flow
(session_to_json/session_to_markdown and send_file) only runs when parsing and
stats succeed.
---
Nitpick comments:
In `@tests/test_api_routes.py`:
- Around line 68-72: The test_search_limit_capped_at_max test currently only
checks len(resp.get_json()) <= 500 which will pass for non-list JSON shapes;
update the test to first assert the response body is a JSON list (e.g., use
resp.is_json and assert isinstance(resp.get_json(), list) or equivalent) before
asserting its length, so the shape is validated prior to the capped-limit check
in test_search_limit_capped_at_max.
In `@tests/test_cli_e2e.py`:
- Around line 50-55: The test test_cli_list_exits_zero uses a loose assertion
that allows header-only output to pass; tighten it by removing the fallback
check ("or 'Project' in proc.stdout") and assert directly that the seeded
project name "test-project" appears in proc.stdout (use the existing proc from
_run_cli in test_cli_list_exits_zero to perform the check).
- Around line 70-73: The test test_cli_invalid_since_exits_nonzero only checks a
non-zero exit code; update it to also assert that _run_cli([...])'s stderr
contains the specific validation error for the --since flag (e.g., assert
"Invalid value for '--since'" in proc.stderr or a substring like "--since" and
"invalid" if exact text differs) so the test verifies the --since validation
path failed; keep using the existing helper _run_cli to capture stdout/stderr
and add the stderr assertion alongside the returncode check.
In `@tests/test_search.py`:
- Around line 8-12: Update the happy-path assertions in
test_limit_integer_string (and the similar test at lines 26-29) to assert the
response is a list, each element has the expected shape (e.g., dict with
expected keys) and that the number of returned items respects the requested or
default limit: assert isinstance(resp.get_json(), list), assert
all(isinstance(item, dict) and set(expected_keys).issubset(item.keys()) for item
in resp.get_json()), and assert len(resp.get_json()) <= requested_limit (for
"/api/search?q=Hello&limit=10" assert <= 10) and also add a case that verifies
the default limit when limit is omitted by calling the endpoint without limit
and asserting len(...) <= default_limit; use the test function names
test_limit_integer_string and the other test to locate and update the
assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93da7cea-ce58-4a41-9562-246a7f450f6a
📒 Files selected for processing (13)
README.mdapi/error_codes.pyapi/export_api.pyapi/search.pyapi/sessions.pytests/conftest.pytests/fixtures/session_minimal.jsonltests/fixtures/session_with_tools.jsonltests/test_api_routes.pytests/test_cli_e2e.pytests/test_error_codes.pytests/test_export_api_bulk.pytests/test_search.py
clean6378-max-it
force-pushed
the
test/http-cli-errors-and-codes
branch
from
20ac332 to
b2f1331
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
api/sessions.py (1)
65-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSplit parse and compute error handling in stats endpoint.
At Line 66 and Line 67, parse and stats computation share one
except, so parse failures are still returned asINTERNAL_ERRORinstead ofPARSE_ERROR.Suggested fix
- try: - session = parse_session(filepath) - stats = compute_stats(session) - return jsonify(stats) - except Exception: - current_app.logger.exception("Failed to compute stats for %s", session_id) - return error_response( - ErrorCode.INTERNAL_ERROR, - "Failed to compute session stats", - 500, - ) + try: + session = parse_session(filepath) + except Exception: + current_app.logger.exception("Failed to parse session %s", session_id) + return error_response( + ErrorCode.PARSE_ERROR, + "Failed to parse session", + 500, + ) + + try: + stats = compute_stats(session) + return jsonify(stats) + except Exception: + current_app.logger.exception("Failed to compute stats for %s", session_id) + return error_response( + ErrorCode.INTERNAL_ERROR, + "Failed to compute session stats", + 500, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/sessions.py` around lines 65 - 75, The endpoint currently wraps parse_session(filepath) and compute_stats(session) in a single try/except so parse failures are treated as INTERNAL_ERROR; split them into two blocks: call parse_session(filepath) inside its own try/except and on parse failure log via current_app.logger.exception("Failed to parse session %s", session_id) and return error_response(ErrorCode.PARSE_ERROR, "Failed to parse session", 400) (catch the specific parse exception class if available, e.g., ParseError or ValueError), then call compute_stats(session) in a separate try/except that logs via current_app.logger.exception("Failed to compute stats for %s", session_id) and returns the existing INTERNAL_ERROR response on other failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/export_api.py`:
- Around line 286-295: Wrap the calls to parse_session(filepath) and
compute_stats(session) in try/except blocks so parsing or stats errors return
structured error_response objects (use the existing error_response and ErrorCode
enums) instead of raising uncaught exceptions; specifically, catch exceptions
from parse_session in the block where parse_session is invoked (before
is_session_excluded) and return a clear error_response (e.g.,
ErrorCode.SESSION_NOT_FOUND or a validation error) with a short message
including the exception text and an appropriate HTTP status, and catch
exceptions from compute_stats(session) where compute_stats is called and return
error_response(ErrorCode.INTERNAL_ERROR, "Failed to compute stats: <error>",
500) so all failures yield stable responses containing the expected code field.
In `@static/js/shared/state.test.js`:
- Around line 5-10: The test suite currently only resets the shared singleton in
an afterEach block, which leaves the first test vulnerable to prior mutations;
add a beforeEach that performs the same resets as the existing afterEach (reset
state.currentProject, state.cachedSessions, state.projectDisplayNames,
state.navInProgress) so each test starts with a deterministic state; locate the
existing afterEach in the file (and the shared state object reference named
state) and duplicate its reset logic inside a beforeEach at the top of the
suite.
In `@tests/test_cli_e2e.py`:
- Around line 70-73: The test test_cli_invalid_since_exits_nonzero is
under-specified because it calls _run_cli(["--since", "yesterday"]) without a
subcommand so a non-zero exit could be from argument shape errors rather than
--since validation; update the test to invoke the CLI with a real subcommand
(use whatever existing subcommand your CLI expects when exercising --since) via
_run_cli, assert proc.returncode != 0, and also assert proc.stderr contains the
specific validation error text related to --since (or a known error fragment) to
ensure the failure is the intended validation path.
---
Duplicate comments:
In `@api/sessions.py`:
- Around line 65-75: The endpoint currently wraps parse_session(filepath) and
compute_stats(session) in a single try/except so parse failures are treated as
INTERNAL_ERROR; split them into two blocks: call parse_session(filepath) inside
its own try/except and on parse failure log via
current_app.logger.exception("Failed to parse session %s", session_id) and
return error_response(ErrorCode.PARSE_ERROR, "Failed to parse session", 400)
(catch the specific parse exception class if available, e.g., ParseError or
ValueError), then call compute_stats(session) in a separate try/except that logs
via current_app.logger.exception("Failed to compute stats for %s", session_id)
and returns the existing INTERNAL_ERROR response on other failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3934581-cfaf-4dc7-841d-4cc84fc0bd73
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
.github/workflows/ci.yml.gitignoreREADME.mdapi/error_codes.pyapi/export_api.pyapi/search.pyapi/sessions.pypackage.jsonpyproject.tomlrequirements-dev.txtstatic/js/shared/markdown.test.jsstatic/js/shared/state.test.jsstatic/js/shared/utils.test.jstests/conftest.pytests/fixtures/session_minimal.jsonltests/fixtures/session_with_thinking.jsonltests/fixtures/session_with_tools.jsonltests/test_api_integration.pytests/test_api_routes.pytests/test_cli_e2e.pytests/test_error_codes.pytests/test_export_api_bulk.pytests/test_search.pyvitest.config.js
✅ Files skipped from review due to trivial changes (4)
- package.json
- pyproject.toml
- .gitignore
- README.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/sessions.py`:
- Around line 67-73: The except block that catches parser exceptions (in the
session handling function around current_app.logger.exception("Failed to parse
session %s", session_id)) returns ErrorCode.PARSE_ERROR with HTTP 400; change
the response status to 500 to match get_session's PARSE_ERROR handling and
maintain a consistent server-error contract. Update the error_response call in
that except handler to use a 500 status, and apply the same change to the
mirrored export-session parse handling so both endpoints return PARSE_ERROR as a
500 server error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8da4a479-8f9a-42d1-9a60-9ef18f6f6cce
📒 Files selected for processing (7)
api/export_api.pyapi/sessions.pytests/conftest.pytests/test_api_routes.pytests/test_cli_e2e.pytests/test_error_propagation.pytests/test_search.py
timon0305
requested changes
May 21, 2026
| status: int, | ||
| **extra: object, | ||
| ) -> tuple[Response, int]: | ||
| body: dict[str, object] = {"error": message, "code": str(code)} |
Collaborator
There was a problem hiding this comment.
ErrorCode is StrEnum, so code is already a str subclass. str(code) works but code.value or just code is more idiomatic — both yield "SEARCH_INVALID_LIMIT" directly. Same nit applies to expected_code=str(code) in tests/test_error_codes.py (in the parametrized assertion at the bottom of the file). Optional.
|
|
||
|
|
||
| @pytest.fixture | ||
| def client(tmp_path, export_state_file): |
Collaborator
There was a problem hiding this comment.
Four near-identical client fixtures (client, client_single, client_empty, client_thinking) repeat the same mkdir / copy / create_app / TESTING boilerplate. Same DRY concern I flagged on PR #40 — still not addressed. Worth collapsing to a parametric helper that takes a list of (fixture_file, output_name) tuples and returns a client. Recommend: a _make_client(tmp_path, export_state_file, seeds: list[tuple[str, str]]) private helper + 4 thin fixture wrappers around it.
| globalThis.marked = { | ||
| parse: vi.fn((text) => `<p>${text}</p>`), | ||
| }; | ||
| globalThis.DOMPurify = { |
Collaborator
There was a problem hiding this comment.
The mock replaces DOMPurify.sanitize with a regex that strips <script>, then the test asserts the output doesn't contain <script>. Same circular-mock issue I flagged on PR #40 — still not addressed.
| 404, | ||
| ) | ||
| return jsonify(session) | ||
| except Exception: |
Collaborator
There was a problem hiding this comment.
Three except Exception: blocks catch too broadly. Same narrowing recommendation as before — (json.JSONDecodeError, KeyError, ValueError, OSError, FileNotFoundError).
Same as line 67, 78.
Collaborator
There was a problem hiding this comment.
Four broad except Exception: sites — the two inside the export loops (162, 224) continue silently and the two below (289, 309) wrap parse/stats failures.
Same as 224, 289, 309
Comment on lines
+235
to
+241
| @@ -226,14 +236,11 @@ def bulk_export(): | |||
| _write_state(new_sessions_map, count) | |||
|
|
|||
| if count == 0: | |||
| return ( | |||
| jsonify( | |||
| { | |||
| "error": "Nothing to export", | |||
| "since": since, | |||
| } | |||
| ), | |||
| return error_response( | |||
| ErrorCode.EXPORT_NOTHING_TO_EXPORT, | |||
| "Nothing to export", | |||
Collaborator
There was a problem hiding this comment.
Reordering to if count == 0: return ... first (then write state for the count > 0 path) reads more linearly. Cosmetic.
Comment on lines
+56
to
+60
| def test_cli_list_unknown_project_exits_zero_with_message(tmp_path): | ||
| base = _seed_base_dir(tmp_path) | ||
| proc = _run_cli(["list", "--base-dir", str(base), "--project", "does-not-exist"]) | ||
| assert proc.returncode == 0 | ||
| assert "No projects found" in proc.stdout |
Collaborator
There was a problem hiding this comment.
Test name says "unknown project" but assertion checks for "No projects found" (about no projects at all). If --project does-not-exist is supposed to show a project-specific "no such project: does-not-exist" message, the test would currently pass even on the wrong message
Comment on lines
+7
to
+12
| omit = [ | ||
| "tests/*", | ||
| "utils/md_exporter.py", | ||
| "utils/session_stats.py", | ||
| "utils/json_exporter.py", | ||
| ] |
Collaborator
There was a problem hiding this comment.
The three utils/ modules omitted from coverage are domain logic, not glue — session_stats.py produces the cost_estimate_usd shown in stats endpoints, md_exporter.py is used by every bulk-export test in this PR, and json_exporter.py likewise.
clean6378-max-it
force-pushed
the
test/http-cli-errors-and-codes
branch
from
ba2117d to
6bb2245
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_search.py (1)
52-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert result-shape/cap for whitespace
limitas well.This test currently verifies only status code, so it won’t catch regressions where whitespace no longer defaults to the capped limit behavior.
Suggested test update
def test_limit_whitespace_defaults(client_single): resp = client_single.get("/api/search?q=Hello&limit=%20%20%20") assert resp.status_code == 200 + _assert_search_hits(resp.get_json(), max_items=50)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_search.py` around lines 52 - 55, The test test_limit_whitespace_defaults only asserts status code; update it to also assert the response result shape and that the returned hits are capped when limit is whitespace — e.g., parse resp.json(), assert it contains a "results" (or "hits") array and that len(resp.json()["results"]) equals the expected default cap (or is <= the configured MAX_LIMIT), using the existing client_single GET to "/api/search?q=Hello&limit=%20%20%20" so regressions in whitespace limit handling are caught.
🧹 Nitpick comments (2)
api/error_codes.py (1)
27-28: ⚡ Quick winProtect reserved response fields from accidental override.
extracurrently overwrites"error"/"code"if those keys are passed, which can silently break the structured-error contract.Proposed hardening
- body: dict[str, object] = {"error": message, "code": str(code)} - body.update(extra) + body: dict[str, object] = {"error": message, "code": str(code)} + for key, value in extra.items(): + if key in {"error", "code"}: + continue + body[key] = value return jsonify(body), status🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/error_codes.py` around lines 27 - 28, The response builder currently does body: dict[str, object] = {"error": message, "code": str(code)} followed by body.update(extra), which allows callers to override the reserved "error" and "code" fields; change the update so reserved keys are protected (e.g., filter out "error" and "code" from the extra mapping before updating or use logic that only adds keys not in {"error","code"}) so that the variables body and extra cannot overwrite the structured fields "error" and "code".static/js/shared/markdown.test.js (1)
42-46: 💤 Low valueConsider adding a DOMPurify fallback test for consistency.
The suite tests the fallback when
markedis unavailable, but there's no parallel test for whenDOMPurifyis missing. Adding one would verify thatrenderMarkdownhandles both missing dependencies gracefully.✨ Optional test to add
After line 46, you could add:
it('handles missing DOMPurify gracefully', () => { delete globalThis.DOMPurify; const html = renderMarkdown('Hello **world**'); // Assert expected fallback behavior based on markdown.js implementation expect(html).toBeDefined(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/js/shared/markdown.test.js` around lines 42 - 46, Add a parallel test that verifies renderMarkdown handles missing DOMPurify similar to the existing marked fallback test: in the test, delete globalThis.DOMPurify, call renderMarkdown with a sample string (e.g., 'Hello **world**' or any markdown input), and assert the returned HTML is defined or matches the expected fallback output; reference the existing test for marked and the renderMarkdown function to mirror setup/teardown and expectations so the suite covers both missing dependencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_cli_e2e.py`:
- Around line 23-36: The helper _run_cli currently calls subprocess.run without
a timeout which can hang CI; update _run_cli to enforce a bounded timeout by
adding a timeout argument (e.g., timeout: float = 30.0) to the function
signature or otherwise passing a fixed timeout value, and pass that timeout into
the subprocess.run(...) call so the CLI process is killed and the test fails
instead of hanging; modify references to _run_cli if you add a parameter and
ensure subprocess.run receives the timeout parameter.
---
Outside diff comments:
In `@tests/test_search.py`:
- Around line 52-55: The test test_limit_whitespace_defaults only asserts status
code; update it to also assert the response result shape and that the returned
hits are capped when limit is whitespace — e.g., parse resp.json(), assert it
contains a "results" (or "hits") array and that len(resp.json()["results"])
equals the expected default cap (or is <= the configured MAX_LIMIT), using the
existing client_single GET to "/api/search?q=Hello&limit=%20%20%20" so
regressions in whitespace limit handling are caught.
---
Nitpick comments:
In `@api/error_codes.py`:
- Around line 27-28: The response builder currently does body: dict[str, object]
= {"error": message, "code": str(code)} followed by body.update(extra), which
allows callers to override the reserved "error" and "code" fields; change the
update so reserved keys are protected (e.g., filter out "error" and "code" from
the extra mapping before updating or use logic that only adds keys not in
{"error","code"}) so that the variables body and extra cannot overwrite the
structured fields "error" and "code".
In `@static/js/shared/markdown.test.js`:
- Around line 42-46: Add a parallel test that verifies renderMarkdown handles
missing DOMPurify similar to the existing marked fallback test: in the test,
delete globalThis.DOMPurify, call renderMarkdown with a sample string (e.g.,
'Hello **world**' or any markdown input), and assert the returned HTML is
defined or matches the expected fallback output; reference the existing test for
marked and the renderMarkdown function to mirror setup/teardown and expectations
so the suite covers both missing dependencies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7dcff1ca-4e45-4767-8c3b-4afdf9b6a02a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
README.mdapi/error_codes.pyapi/export_api.pyapi/search.pyapi/sessions.pypackage.jsonstatic/js/shared/markdown.test.jsstatic/js/shared/state.test.jstests/conftest.pytests/test_api_integration.pytests/test_api_routes.pytests/test_cli_e2e.pytests/test_error_codes.pytests/test_error_propagation.pytests/test_export_api_bulk.pytests/test_search.py
✅ Files skipped from review due to trivial changes (1)
- README.md
clean6378-max-it
added a commit
that referenced
this pull request
May 22, 2026
timon0305
approved these changes
May 22, 2026
Extract Flask client fixtures to tests/conftest.py, use client_single for search limit tests, and relax session count assertion to >= 1. Include utils/ in coverage (omit untested export modules), stop tracking .coverage, and align integration-tests CI with full cov scope. Co-Authored-By: Cursor <cursoragent@cursor.com>
- Add ErrorCode enum and error_response() helper; migrate api/search, api/sessions, and api/export_api error paths to include stable "code" - Validate search limit query param (400 SEARCH_INVALID_LIMIT; cap at 500) - Add tests/conftest.py, fixtures, test_api_routes, test_cli_e2e, test_error_codes, test_search; extend test_export_api_bulk for codes - Document error code catalog in README
clean6378-max-it
force-pushed
the
test/http-cli-errors-and-codes
branch
from
fbec114 to
5daf230
Compare
wpak-ai
approved these changes
May 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #41
Summary
Week 3 Wednesday work for
claude-code-chat-browser(8 story points):test_api_routes.py), shared fixtures (tests/conftest.py), CLI subprocess e2e (test_cli_e2e.py), and search limit unit tests (test_search.py).ErrorCodeenum +error_response()helper; every API JSON error includes"code"and"error"; README error catalog.Canonical issues:
fab633e4-4f9d-56b4-97d4-cfc84b5bbdd7(HTTP/CLI tests) ·8c01c675-d0e7-5e5c-a179-472a2fea5056(structured error codes)Detail guide:
Doc/Issues/chen-week3-wednesday-claude-code-chat-browser-guide.mdChanges
API
api/error_codes.pyErrorCode(StrEnum) anderror_response()api/search.py_parse_limit(); 400SEARCH_INVALID_LIMIT; max limit 500api/sessions.pyINVALID_PATH,SESSION_NOT_FOUND,PARSE_ERROR,INTERNAL_ERROR)api/export_api.pyTests
tests/conftest.pyclientfixtures,export_state_file,assert_error_response()tests/fixtures/*.jsonltests/test_api_routes.pytests/test_cli_e2e.pylist,stats,export, invalid--since(UTF-8 env for Windows)tests/test_error_codes.py"code"field assertionstests/test_search.pySEARCH_INVALID_LIMITtests/test_export_api_bulk.py"code"on isolated blueprint testsDocs
README.md— API error code table; bulk export 422 example includes"code"Example error response
{ "error": "Invalid limit: must be a positive integer", "code": "SEARCH_INVALID_LIMIT" } <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * API error responses now include stable, machine-readable error codes alongside human messages. * **Documentation** * Bulk Export docs expanded; new API error codes section and standardized JSON error shape documented. * **Bug Fixes** * Unified, validated error handling across search, export, and session endpoints (including explicit errors for invalid parameters, path issues, parse failures, and a 422 when bulk export yields nothing). * **Tests** * Added extensive unit, integration, and end-to-end tests for API, CLI, and frontend utilities. * **Chores** * CI and JS test tooling configuration updates. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/cppalliance/claude-code-chat-browser/pull/42?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->